Add mobile skeleton loading for Home Screen groups #279
Conversation
Co-authored-by: Devasy23 <110348311+Devasy23@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for split-but-wiser canceled.
|
WalkthroughImplements mobile skeleton loading UI for the HomeScreen group list. Creates a reusable Skeleton component with pulsing animation and a GroupListSkeleton component that renders placeholder cards. Replaces the ActivityIndicator loading indicator with the skeleton placeholder. Updates documentation to reflect completion. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mobile/components/skeletons/GroupListSkeleton.js`:
- Around line 14-30: Update the static Skeleton dimensions in the
GroupListSkeleton component so each Card renders varied widths: define small
arrays like titleWidths = [120,150,100,130,110] and subtitleWidths =
[200,160,180,140,170] and pick values by index inside the map that renders Card
and Skeleton (references: the map over [...Array(5)], Card.Title, Card.Content,
and Skeleton). Replace the hardcoded numeric props (title: 120, subtitle: 200)
with titleWidths[index] and subtitleWidths[index] (or a simple deterministic
alternation) so the skeletons vary while keeping layout stable.
In `@mobile/components/ui/Skeleton.js`:
- Around line 9-24: The Animated.loop started in the useEffect (which sequences
Animated.timing on opacity) isn’t stopped on unmount; capture the loop
instance/animation returned by Animated.loop(...) (or the value returned by
.start()) and return a cleanup function from useEffect that calls .stop() on
that instance so the looping animation is properly stopped when the component
unmounts; reference the useEffect, Animated.loop, opacity, .start(), and .stop()
in your change.
| {[...Array(5)].map((_, index) => ( | ||
| <Card key={index} style={styles.card}> | ||
| <Card.Title | ||
| title={<Skeleton width={120} height={20} />} | ||
| left={(props) => ( | ||
| <Skeleton | ||
| width={props.size} | ||
| height={props.size} | ||
| borderRadius={props.size / 2} | ||
| /> | ||
| )} | ||
| /> | ||
| <Card.Content> | ||
| <Skeleton width={200} height={16} style={styles.subtitle} /> | ||
| </Card.Content> | ||
| </Card> | ||
| ))} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider varying skeleton widths for a more natural look.
All five cards render identical skeleton dimensions (title: 120, subtitle: 200). Slightly randomizing or alternating widths (e.g., [120, 150, 100, 130, 110]) would produce a more realistic shimmer effect and avoid a rigid, repetitive appearance.
🤖 Prompt for AI Agents
In `@mobile/components/skeletons/GroupListSkeleton.js` around lines 14 - 30,
Update the static Skeleton dimensions in the GroupListSkeleton component so each
Card renders varied widths: define small arrays like titleWidths =
[120,150,100,130,110] and subtitleWidths = [200,160,180,140,170] and pick values
by index inside the map that renders Card and Skeleton (references: the map over
[...Array(5)], Card.Title, Card.Content, and Skeleton). Replace the hardcoded
numeric props (title: 120, subtitle: 200) with titleWidths[index] and
subtitleWidths[index] (or a simple deterministic alternation) so the skeletons
vary while keeping layout stable.
| useEffect(() => { | ||
| Animated.loop( | ||
| Animated.sequence([ | ||
| Animated.timing(opacity, { | ||
| toValue: 1, | ||
| duration: 1000, | ||
| useNativeDriver: true, | ||
| }), | ||
| Animated.timing(opacity, { | ||
| toValue: 0.5, | ||
| duration: 1000, | ||
| useNativeDriver: true, | ||
| }), | ||
| ]) | ||
| ).start(); | ||
| }, [opacity]); |
There was a problem hiding this comment.
Missing animation cleanup on unmount.
The looping animation is never stopped when the component unmounts, which can cause a memory leak and "update on unmounted component" warnings. Store the return value of Animated.loop(…).start() — or the loop reference — and call .stop() in the effect cleanup.
🛡️ Proposed fix
useEffect(() => {
- Animated.loop(
+ const animation = Animated.loop(
Animated.sequence([
Animated.timing(opacity, {
toValue: 1,
duration: 1000,
useNativeDriver: true,
}),
Animated.timing(opacity, {
toValue: 0.5,
duration: 1000,
useNativeDriver: true,
}),
])
- ).start();
+ );
+ animation.start();
+ return () => animation.stop();
}, [opacity]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| Animated.loop( | |
| Animated.sequence([ | |
| Animated.timing(opacity, { | |
| toValue: 1, | |
| duration: 1000, | |
| useNativeDriver: true, | |
| }), | |
| Animated.timing(opacity, { | |
| toValue: 0.5, | |
| duration: 1000, | |
| useNativeDriver: true, | |
| }), | |
| ]) | |
| ).start(); | |
| }, [opacity]); | |
| useEffect(() => { | |
| const animation = Animated.loop( | |
| Animated.sequence([ | |
| Animated.timing(opacity, { | |
| toValue: 1, | |
| duration: 1000, | |
| useNativeDriver: true, | |
| }), | |
| Animated.timing(opacity, { | |
| toValue: 0.5, | |
| duration: 1000, | |
| useNativeDriver: true, | |
| }), | |
| ]) | |
| ); | |
| animation.start(); | |
| return () => animation.stop(); | |
| }, [opacity]); |
🤖 Prompt for AI Agents
In `@mobile/components/ui/Skeleton.js` around lines 9 - 24, The Animated.loop
started in the useEffect (which sequences Animated.timing on opacity) isn’t
stopped on unmount; capture the loop instance/animation returned by
Animated.loop(...) (or the value returned by .start()) and return a cleanup
function from useEffect that calls .stop() on that instance so the looping
animation is properly stopped when the component unmounts; reference the
useEffect, Animated.loop, opacity, .start(), and .stop() in your change.
1 participant
Implemented a skeleton loading state for the mobile Home Screen to improve the perceived performance and user experience.
Features:
Skeletonprimitive component inmobile/components/ui/Skeleton.jsthat supports pulsing animations and theme adaptation.GroupListSkeletoninmobile/components/skeletons/GroupListSkeleton.jsthat mimics the layout of the group list cards.ActivityIndicatorinmobile/screens/HomeScreen.jswith the new skeleton loading state.Changes:
mobile/components/ui/Skeleton.js: New component.mobile/components/skeletons/GroupListSkeleton.js: New component.mobile/screens/HomeScreen.js: Replaced loader with skeleton, removed unused styles..Jules/todo.md: Marked task as complete..Jules/changelog.md: Added entry for the new feature.PR created automatically by Jules for task 3504540643638496533 started by @Devasy23
Summary by CodeRabbit